fix(dash): detect rocm serve's default port in chat auto-detect - #88
Conversation
The TUI chat's local-engine TCP-probe fallback only checked Lemonade's :13305 and vLLM's :8000, so it never found vLLM instances started via `rocm serve <model> --engine vllm` in non-managed mode, which listen on rocm-serve's own default port :11435. Add :11435 as a third probe candidate and update the UI copy that hardcoded the old two-port list. EAI-7220 Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
michaelroy-amd
left a comment
michaelroy-amd
left a comment
There was a problem hiding this comment.
Review — Approve ✅
Small, correct, well-scoped fix. Core claims verified against the codebase.
Verified
- Port value is correct:
rocm_core::DEFAULT_LOCAL_PORT = 11_435(crates/rocm-core/src/lib.rs:65) — the mirrored literal matches exactly. - No-dependency rationale is real:
rocm-dash-tui/Cargo.tomlgenuinely has norocm-coredep, so duplicating the literal (with a comment pointing back to the source of truth) is consistent with the existing pattern inserve_wizard.rs. - Detect path is clean:
detect_local_chat→detect_local_endpoint→detected_llm_config(llm.rs:171), which carries onlybase_url/model/auth (no engine classification), so surfacing:11435here needs nothing more. llm::tests pass; UI copy strings updated in lockstep; doc-comment first paragraph stays short (respects the repo'stoo_long_first_doc_paragraphclippy lint).
Notes (non-blocking)
LOW — latent engine-inference gap. auto_config_change (skills.rs:227) infers default_engine only from LEMONADE_PORT/VLLM_PORT; a :11435 endpoint would fall into the else → "local", even though rocm serve is vLLM. This is not on this PR's path, and auto_config_plan/auto_config_change currently have no runtime caller (tests only), so it's dormant — not a regression. Since this PR establishes ":11435 = vLLM", adding an ROCM_SERVE_PORT arm there would prevent the two mappings from drifting when that skill is eventually wired up. Fine to defer.
LOW — test isolation. detect_local_endpoint_probes_rocm_serve_default_port skips if :11435 is already bound, but doesn't guard against :13305/:8000 being occupied on the test host. Since the probe returns the first reachable endpoint in order, a stray listener on those ports would make the assert_eq! fail rather than skip. Low real-world risk in CI; the skip-on-bind instinct is right.
3 participants
Summary
rocm servestarts vLLM on its own default port (11435,rocm_core::DEFAULT_LOCAL_PORT), but the dash chat tab's TCP-probe fallback (used when noManagedServiceRecordexists, i.e.rocm servewithout--managed) only checked:8000(vLLM's own default) and:13305(Lemonade).rocm-dash-tuideliberately carries norocm-coredependency, same pattern already used inserve_wizard.rs), and updates the two UI copy strings that hardcoded the old port list so they stay in sync.Test plan
cargo testpasses forrocm-dash-tuicargo clippy --workspace --all-targets -- -D warningspassescargo fmt --checkpasses